[rom_ctrl,dv] Rewrite a rom_ctrl vseq to avoid tl_access_sub#29942
[rom_ctrl,dv] Rewrite a rom_ctrl vseq to avoid tl_access_sub#29942rswarbrick wants to merge 1 commit into
Conversation
rswarbrick
requested review from
hcallahan-lowrisc
and removed request for
a team
rswarbrick
force-pushed
the
rom-ctrl-tl-access-sub
branch
from
f93b922 to
3288935
Compare
|
Force-push rebases and addresses most of the comments above. |
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
Some comments mainly for me to understand the change better.
| // an integrity error. | ||
| if (seq.rsp.d_error) begin | ||
| `uvm_error(`gfn, $sformatf("Unexpected error reading from address 0x%0h", addr)) | ||
| end |
There was a problem hiding this comment.
Nit: should we have an empty line between end and if?
There was a problem hiding this comment.
Good point. In that version, I was grouping the two checks together as "the checks to do when the sequence has completed". But that's rather ugly. I've just split them up and also (I think) improved the comments.
| task rom_ctrl_corrupt_sig_fatal_chk_vseq::corrupt_rom_address(); | ||
| addr_range_t loc_mem_range[$] = cfg.ral_models["rom_ctrl_prim_reg_block"].mem_ranges; | ||
| dv_base_reg_block blk = cfg.ral_models["rom_ctrl_prim_reg_block"]; | ||
| int mem_idx = $urandom_range(0, blk.mem_ranges.size - 1); |
There was a problem hiding this comment.
Do you mean mem_idx? It's used as the index for the array access on the next line.
| cfg.rom_ctrl_vif.override_bus_rom_index(corr_bus_rom_rom_index_val); | ||
| end | ||
| read_word(addr, corr_data, 1'b0); | ||
| cfg.rom_ctrl_vif.override_bus_rom_index(corr_bus_rom_rom_index_val); |
There was a problem hiding this comment.
Where is this index set?
There was a problem hiding this comment.
Oops! Nowhere! Thanks for noticing.
| cfg.rom_ctrl_vif.override_bus_rom_index(corr_bus_rom_rom_index_val); | ||
| end | ||
| read_word(addr, corr_data, 1'b0); | ||
| cfg.rom_ctrl_vif.override_bus_rom_index(corr_bus_rom_rom_index_val); |
There was a problem hiding this comment.
Why don't we override the index before the read?
There was a problem hiding this comment.
Ah: the problem is that override_bus_rom_index consumes time: it waits until a_valid is asserted and then forces the index until a_valid drops again.
Looking closely at the code (dating back to the original version in 0c4edc1), it does assume that there won't be any other bus accesses already in flight: it assumes that when a_valid is asserted, that must be the word we're trying to read.
That's probably not unreasonable for this FI test though, I hope.
There was a problem hiding this comment.
I thought about this a bit in bed(!) and wonder whether the name of this task in the interface could be better. But I'm struggling to think of something short and clear. The best I could come up with was override_bus_rom_index_during_request, but I'm not really convinced. Any ideas?
Another idea: override_bus_rom_index_throughout_request.
rswarbrick
force-pushed
the
rom-ctrl-tl-access-sub
branch
from
3288935 to
86ea6b7
Compare
This task was really intended to be internal, so maybe it's better not to use it directly. Upsettingly, avoiding that means you have to duplicate a bit of its contents, but that turns out to be cleaner than I feared. I've also expanded out the error messages and (hopefully) made it a bit more obvious what the sequence is doing. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
rswarbrick
force-pushed
the
rom-ctrl-tl-access-sub
branch
from
86ea6b7 to
b0e84ef
Compare
|
The double force-push is to rebase onto master. If comparing versions, the first "compare" button (here) points at the changes I've made to respond to the review. The second compare button is just the enormous changes from a rebase. |
3 participants
This task was really intended to be internal, so maybe it's better not to use it directly. Upsettingly, avoiding that means you have to duplicate a bit of its contents, but that turns out to be cleaner than I feared.
I've also expanded out the error messages and (hopefully) made it a bit more obvious what the sequence is doing.